Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for "." in pathnames #132 #133

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

patrikpessi
Copy link

@lukeed
Copy link
Owner

lukeed commented Feb 9, 2022

This is just allowing numbers with periods, specific to your use case

@patrikpessi
Copy link
Author

I wouldn't say this is specific for my project since some packages (eg. express) seem to handle such routes aswell.
Working example using express:

const fs = require("fs")
const express = require("express")
const app = express()

app.use(express.static("public"))

app.get("*", (req, res) => {
    const html = fs.readFileSync("public/index.html")
    res.send(html.toString())
})

app.listen(3000)

@lukeed
Copy link
Owner

lukeed commented Feb 9, 2022

Right, I’m not saying the need is specific to you (should be done) but this fix is specifically only allowing paths with dot-separated numbers, which is your use case.

@patrikpessi
Copy link
Author

I tested it on regexr and it should also allow paths with word characters aswell.
Here are the tests I used. Please let me know if there is some edge case here that I'm missing?

@lukeed
Copy link
Owner

lukeed commented Feb 22, 2022

It stops matching URLs like /foo/bar.mp3 and /foo/bar.txt.mp3 because there's a digit in the final extension segment. This is what I meant originally – the RegExp is now specifically targeting digits, which is your use case

@patrikpessi
Copy link
Author

Updated the regex for better file name/extension matching
Here is another link to tests used for the regex: https://regexr.com/6fv5m

@patrikpessi
Copy link
Author

@lukeed Toughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants